Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[oneMKL] Rename oneMKL Interface project #596

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

s-Nick
Copy link

@s-Nick s-Nick commented Oct 17, 2024

This PR address RFC #580.
It performs all the changes to namespace, name and file name requested in the issue.

EDIT:
For completeness, I attach a zip of the html version with changes
oneMath_html.zip

@s-Nick s-Nick requested a review from Rbiessy October 17, 2024 12:28
@Rbiessy Rbiessy requested a review from a team October 17, 2024 13:10
@@ -11,8 +11,8 @@ BLAS-like Extensions
.. container::


oneAPI Math Kernel Library DPC++ provides additional routines to
extend the functionality of the BLAS routines. These include routines
oneAPI Math Library DPC++ provides additional routines to extend
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have |onemath_full_name|m should we replace here: |onemath_full_name| (oneMath) ? and maybe look for it elsewhere ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated them in 19c21e4 and, as far as I can tell, there aren't other occurrences of the full name

@s-Nick
Copy link
Author

s-Nick commented Oct 22, 2024

Hi @rscohn2, these changes are failing pre-commit checks due to two .jpg files. What should I do about it?

@rscohn2
Copy link
Member

rscohn2 commented Oct 22, 2024

Hi @rscohn2, these changes are failing pre-commit checks due to two .jpg files. What should I do about it?

Add it here: https://github.com/uxlfoundation/oneAPI-spec/blob/main/.reuse/dep5

- Move source/elements/oneMKL to source/elements/oneMath
- Rename files using "mkl" or "onemkl" to "onemath"
- Rename occurrences of "oneMKL" and "onemkl" to "oneMath" and "onemath"
  respectively

Signed-off-by: nscipione <[email protected]>
- Rename the namespace oneapi::mkl to oneapi::math and references to
  the folder oneapi/mkl to oneapi/math

Signed-off-by: nscipione <[email protected]>
- Rename remaining occurrences of "mkl" to "onemath"

Signed-off-by: nscipione <[email protected]>
Fix table format

Signed-off-by: nscipione <[email protected]>
Replace wrong usage of "onemath" as namespace in favor of the correct
"math"

Signed-off-by: nscipione <[email protected]>
Set full name to "oneAPI Math Library" and update reference to it

Signed-off-by: nscipione <[email protected]>
Rename macros for domain and for version

Signed-off-by: nscipione <[email protected]>
Copy link
Contributor

@Rbiessy Rbiessy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me!
For future reference I am also adding the link to the main RFC: oneapi-src/oneMKL#564

Copy link

@anantsrivastava30 anantsrivastava30 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed fo FFT and approved.

Comment on lines +78 to +81
To help the calling application, all oneMath routines with at least one USM pointer argument also take an optional reference to a list of *input events*, of type ``std::vector<sycl::event>``, and have a return value of type ``sycl::event`` representing computation completion::

sycl::event math::domain::routine(..., std::vector<sycl::event> &in_events = {});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to add const to the front of these types: const std::vector<sycl::event>&

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, that looks like a small fix that can be done as part of this PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I hesitated to toss it into here, but is small enough to just make the change ...

Copy link
Contributor

@spencerpatty spencerpatty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reviewed the architecture + appendix + domains/spblas sections and approve of these changes

@Rbiessy
Copy link
Contributor

Rbiessy commented Nov 26, 2024

@sknepper I see you have approved the renaming of the implementation but not this specification PR. Just pinging you here in case one of the domain owners of lapack want to approve this PR before November 29.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants